Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rsem-calculate-expression #93

Merged
merged 25 commits into from
Sep 18, 2024
Merged

Rsem-calculate-expression #93

merged 25 commits into from
Sep 18, 2024

Conversation

emmarousseau
Copy link
Collaborator

@emmarousseau emmarousseau commented Jul 24, 2024

Description

RSEM-calculate-expression component

Checklist before requesting a review

  • I have performed a self-review of my code

  • Conforms to the Contributing guidelines

  • Proposed changes are described in the CHANGELOG.md

  • I have tested my code with viash ns test --parallel -q <name or namespace>

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Documentation
    • Bug fixes

@rcannood
Copy link
Contributor

Hi @emmarousseau ! Does this PR need more work or is it ready for review? ☺️

@emmarousseau
Copy link
Collaborator Author

Hi @emmarousseau ! Does this PR need more work or is it ready for review? ☺️

Ready now! Let me know if the test data is fine or too big.

@emmarousseau emmarousseau marked this pull request as ready for review August 21, 2024 18:48
@emmarousseau emmarousseau requested a review from rcannood August 21, 2024 18:48
@@ -0,0 +1,23 @@
#!/bin/bash

wget wget https://raw.githubusercontent.com/nf-core/test-datasets/rnaseq3/reference/rsem.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yo dawg I heard you like wget so I put some wget in your wget

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is indeed too big 😅

In other components, I just store the fasta and gtf in the test data and build the index inside the unit test. Examples:

This way, we don't need to store a large indexed reference. Could you get this to work here as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be empty

src/rsem/rsem_calculate_expression/test.sh Outdated Show resolved Hide resolved
src/rsem/rsem_calculate_expression/test_data/script.sh Outdated Show resolved Hide resolved
src/rsem/rsem_calculate_expression/test.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor requests!

🤞 Should be good to merge afterwards

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is no longer needed, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the fastq files are inside test.sh, perhaps store the expected output in test.sh as well? ☺️

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep makes sense haha

Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rcannood rcannood merged commit 619f1bb into viash-hub:main Sep 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants